This (together with colcon/colcon-ros#86) fails to build a ROS 2 workspace for me:

Traceback (most recent call last):
  File "/home/dthomas/colcon/ws/build/colcon-core/colcon_core/executor/__init__.py", line 91, in __call__
    rc = await self.task(*args, **kwargs)
  File "/home/dthomas/colcon/ws/build/colcon-core/colcon_core/task/__init__.py", line 92, in __call__
    return await task_method(*args, **kwargs)
  File "/home/dthomas/colcon/ws/build/colcon-ros/colcon_ros/task/ament_python/build.py", line 97, in build
    return await extension.build(additional_hooks=additional_hooks)
  File "/home/dthomas/colcon/ws/build/colcon-core/colcon_core/task/python/build.py", line 82, in build
    self._symlinks_in_build(args, setup_py_data)
  File "/home/dthomas/colcon/ws/build/colcon-core/colcon_core/task/python/build.py", line 188, in _symlinks_in_build
    if package in package_dir:
TypeError: argument of type 'NoneType' is not iterable

How does this always re-instantiated multiprocessing pool perform compared to a plain subprocess invocation in terms of runtime?

Significant, but not crazy. It's 4x slower than spawning a Python interpreter in a new process, but if you reuse the process pool, it's 7x faster.

call 1.0819000000009127e-07
subprocess 0.00517880357
interpreter 0.0331634016
transient pool 0.11082568893
shared pool 0.004250488220000008

import subprocess
import multiprocessing
import gc
from timeit import timeit

def f0(): return True
def f1(): subprocess.call(['true'])
def f2(): subprocess.call(['python3', '-c', 'True'])
def f3():
    with multiprocessing.Pool(1) as pool: pool.apply(f1)
def f4(p): p.apply(f1)

setup = 'gc.enable()'
n=100
print('call', timeit('f0()', setup, number=n, globals=globals())/n)
print('subprocess', timeit('f1()', setup, number=n, globals=globals())/n)
print('interpreter', timeit('f2()', setup, number=n, globals=globals())/n)
print('transient pool', timeit('f3()', setup, number=n, globals=globals())/n)
with multiprocessing.Pool(1) as pool:
    print('shared pool', timeit('f4(pool)', setup, number=n,
        globals=globals())/n)

It's 4x slower than spawning a Python interpreter in a new process, but if you reuse the process pool, it's 7x faster.

Since the patch currently doesn't reuse the process pool how about keep using subprocess as it is right now in the target branch?

What like about the process pool is it handles return values and exceptions. Plus passing in a process pool can speed up this code as well as the Python project build code.

I think it’s worth it to keep using ProcessPool, even if it is a bit slower for now. Premature optimization and whatnot.

Plus passing in a process pool can speed up this code as well as the Python project build code.

Can you describe how you think the process pool can be used in the future?

Premature optimization and whatnot.

This isn't premature optimization. The target branch uses subprocess and the proposed change switches it to a one-time process pool which is a regression in performance (I might need to measure how much it changes). So at the moment using the process pool provides only some convenience advantage which isn't a good argument for the performance impact (assuming it is not insignificant).

Can you describe how you think the process pool can be used in the future?

Sure. Passing in a process pool to PythonBuildTask and PythonTestTask prevents the need to spin up a process and (more significantly) the Python interpreter for each invocation. This could be created in the context and persisted for the entire run of colcon.

This isn't premature optimization.

How noticeable is the change in performance? I actually observe a speedup with colcon list with the unoptimised new version:

before:
colcon list 2.74s user 0.57s system 86% cpu 3.822 total

after:
colcon list 2.52s user 0.42s system 90% cpu 3.257 total

I need to redo these tests. I'm not sure they captured the intended code path

I compared the timing of colcon list for a larger ROS 2 workspace containing several Python packages and this approach seems to take pretty much the same time as the existing one. So I am ok to merge this when you are.

After one last fix (see prev commit), I'm ready to merge this.
If performance is a problem, #25 will make it faster, though still not as fast as reusing a process pool.

The target branch of this PR only existed while we worked on different approaches to get the information from Python setup files. In the meantime #30 has been merged so I don't see how the current patch can be applied anymore.

Therefore I will go ahead and close this ticket. If necessary please consider creating a new pull request targeting master for any left over desired changes.

Good move. I support closing this PR.